Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add View Option for raw/unadjusted trace #153

Merged
merged 4 commits into from Dec 24, 2017
Merged

Add View Option for raw/unadjusted trace #153

merged 4 commits into from Dec 24, 2017

Conversation

yurishkuro
Copy link
Member

Fixes #152

Add View Option for raw/unadjusted trace
336a570 
Signed-off-by: Yuri Shkuro <ys@uber.com>
yurishkuro
yurishkuro commented Dec 23, 2017
View reviewed changes
src/components/TracePage/TracePageHeader.js Outdated
@@ -104,7 +104,12 @@ export default function TracePageHeader(props: TracePageHeaderProps) {
<Dropdown.Menu>
<Dropdown.Item>
<a rel="noopener noreferrer" target="_blank" href={`/api/traces/${traceID}`}>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffon I don't see this file in #151, it probably needs adjustment with the prefix as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro Yes, this needs the prefixUrl(...).

Also, is the trailing / in the raw version URL intentional?

/api/traces/${traceID}/?raw=true

Seems like the only difference should be the query string is appended?

/api/traces/${traceID}?raw=true

@codecov
Copy link

codecov bot commented Dec 23, 2017 •
edited

Codecov Report

Merging #153 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #153   +/-   ##
=======================================
  Coverage   92.66%   92.66%           
=======================================
  Files          85       85           
  Lines        1881     1881           
  Branches      366      366           
=======================================
  Hits         1743     1743           
  Misses        126      126           
  Partials       12       12
Impacted Files Coverage Δ
src/components/TracePage/TracePageHeader.js 92.85% <ø> (ø) ⬆️
src/components/SearchTracePage/TraceSearchForm.js 86.51% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d673fa...fa904d1. Read the comment docs.

@black-adder
Copy link
Contributor

add this to the change log so it can go out with 1.1?

tiffon
tiffon suggested changes Dec 23, 2017
View reviewed changes
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting change of raw URL to not the trailing slash .../${traceID}/ or confirmation that it is intentional.

Yuri Shkuro and others added 3 commits December 23, 2017 19:15
Remove /
575114d 
Signed-off-by: Yuri Shkuro <ys@uber.com>
@tiffon
Add missing prefixUrl adjustment
27c168a 
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Fix failing unit test
fa904d1 
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Copy link
Member

tiffon commented Dec 24, 2017

@yurishkuro The prefixUrl(...) needs to be in master in the event of package.json#homepage is changed; it's not just relevant to the query-config PR (#151).

I went ahead and added it (and fixed a failing unit test).

@tiffon tiffon merged commit 0e00d93 into master Dec 24, 2017
@yurishkuro yurishkuro deleted the fix-152-trace-json-raw branch January 29, 2020 15:04
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
@yurishkuro @tiffon
Add View Option for raw/unadjusted trace (jaegertracing#153)
ba8c097 
* Add View Option for raw/unadjusted trace

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Remove /

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Add missing prefixUrl adjustment

Signed-off-by: Joe Farro <joef@uber.com>

* Fix failing unit test

Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiffon tiffon tiffon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yurishkuro @black-adder @tiffon